@andyleejordan Can you please review this one?

@Fantoom do you think the CI failure is related to #2170?

Looks like the same issue. I have a hypothesis (not yet checked with the sources): what if a test executes some operation that would cause that exception, but it is currently masked by the fact that PSES doesn't re-evaluate the variables after each step?

In that case, it is possible that there's a real error that is hidden from the test.

@andyleejordan

@Fantoom do you think the CI failure is related to #2170?

I have debugged it and have found that after SetVariable, FetchStackFramesAndVariablesAsync fetches variables with two more variables called "$__psEditorServices_CallStack." And that changes the referenceIds of the scopes.

I see two ways of solving this:

1. Users should clear their own cache after SetValue/Evaluate and refetch with GetVariables. I'm not sure if this violates the DAP or not
2. Update variable value in the cache instead of refetching it. I am not sure if this reflects all the changes or not

After a bit of additional digging (disclaimer: I am a supervisor of @Fantoom), we found these lines in the DAP spec:

Some complex structural objects such as scopes or variables are not returned directly with their containers (stack frames, scopes, variables), but must be retrieved with separate scopes and variables requests based on object references. An object reference is an integer in the open interval (0, 231) assigned to objects by the debug adapter.

To simplify the management of object references in debug adapters, their lifetime is limited to the current suspended state. Once execution resumes, object references become invalid and DAP clients must not use them. When execution is paused again, object references no longer refer to the same objects. This means that a debug adapter can easily use sequentially assigned integers for different objects and reset the counter to 1 when execution resumes.

Variable references not related to the current suspended state, such as those from evaluate requests or in output events, should be preserved as long as feasible for the debug adapter so that the client may later inspect them.

Please note that other object references like threadId do not have limited lifetime because that would prevent something like the pause request from working in the running state.

I am not very sure, but to me it looks like, unfortunately, this PR breaks the spec expectation that the variable ids are preserved between the suspend points (and effectively introduces a new suspend state after any variable evaluation).

I'd rely on your judgement, folks, on what to do with that further. We might have to redesign this part, to allow some kind of partial update of the suspend state, preserving the variable ids?

@ForNeVeR I have this on my radar, just getting back to work after breaking my hand! Will reply more in depth soon.

Rebasing to re-run tests, should be OK now.

After a bit of additional digging (disclaimer: I am a supervisor of @Fantoom), we found these lines in the DAP spec:

Some complex structural objects such as scopes or variables are not returned directly with their containers (stack frames, scopes, variables), but must be retrieved with separate scopes and variables requests based on object references. An object reference is an integer in the open interval (0, 231) assigned to objects by the debug adapter.
To simplify the management of object references in debug adapters, their lifetime is limited to the current suspended state. Once execution resumes, object references become invalid and DAP clients must not use them. When execution is paused again, object references no longer refer to the same objects. This means that a debug adapter can easily use sequentially assigned integers for different objects and reset the counter to 1 when execution resumes.
Variable references not related to the current suspended state, such as those from evaluate requests or in output events, should be preserved as long as feasible for the debug adapter so that the client may later inspect them.
Please note that other object references like threadId do not have limited lifetime because that would prevent something like the pause request from working in the running state.

I am not very sure, but to me it looks like, unfortunately, this PR breaks the spec expectation that the variable ids are preserved between the suspend points (and effectively introduces a new suspend state after any variable evaluation).

I'd rely on your judgement, folks, on what to do with that further. We might have to redesign this part, to allow some kind of partial update of the suspend state, preserving the variable ids?

@JustinGrote don't forget this part!

Sorry, I meant to say the test should be fine, not that the pr is ready to be merged